Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better documentation #293

Merged
merged 54 commits into from
Apr 27, 2020
Merged

Conversation

HalidOdat
Copy link
Member

@HalidOdat HalidOdat commented Apr 1, 2020

This is for improving the documentation of the boa project.

Any feedback on how we can improve the documentation of the code, would be very helpful.

Syntax

  • lexer
  • ast
    • token
    • constant
    • keyword
    • node
      • Node
      • MethodDefinitionKind
      • PropertyDefinition
    • op
      • NumOp
      • UnaryOp
      • BitOp
      • CompOp
      • LogOp
      • AssignOp
      • BinOp
    • pos
    • punc

Builtins

  • array
  • boolean
  • function
  • math
  • number
  • object
  • regex
  • string
  • symbol
  • value
  • console
  • error
  • json
  • property

@jasonwilliams
Copy link
Member

jasonwilliams commented Apr 1, 2020

On a similar note I would be interested in having a blog on here, with the typical rust theme (But it doesn’t have to be) that can be statically generated. Then shown from github.io

Right now I’m using my own site.

I’ll make an issue for it.

@jasonwilliams
Copy link
Member

These additions look good, are there any other areas you wanted help on or planned to fill in?

@HalidOdat
Copy link
Member Author

These additions look good, are there any other areas you wanted help on or planned to fill in?

I would like to document all of syntax module before merging.

@Razican
Copy link
Member

Razican commented Apr 1, 2020

On a similar note I would be interested in having a blog on here, with the typical rust theme (But it doesn’t have to be) that can be statically generated. Then shown from github.io

Right now I’m using my own site.

I’ll make an issue for it.

We can add automatic documentation generation to the GitHub pipelines that upload the docs to GitHub pages.

@HalidOdat
Copy link
Member Author

On a similar note I would be interested in having a blog on here, with the typical rust theme (But it doesn’t have to be) that can be statically generated. Then shown from github.io
Right now I’m using my own site.
I’ll make an issue for it.

We can add automatic documentation generation to the GitHub pipelines that upload the docs to GitHub pages.

Thats amazing! I did not know that such a feature existed.

@jasonwilliams
Copy link
Member

We can add automatic documentation generation to the GitHub pipelines that upload the docs to GitHub pages.

We already have the docs.rs documentation which automatically generates on every release, I don’t understand what the github pages would add? I may be missing the idea here though.

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very nice :) Thanks for the work!

boa/src/syntax/ast/op.rs Outdated Show resolved Hide resolved
boa/src/syntax/ast/op.rs Outdated Show resolved Hide resolved
boa/src/syntax/ast/op.rs Outdated Show resolved Hide resolved
boa/src/syntax/ast/op.rs Outdated Show resolved Hide resolved
@Razican
Copy link
Member

Razican commented Apr 1, 2020

We already have the docs.rs documentation which automatically generates on every release, I don’t understand what the github pages would add? I may be missing the idea here though.

docs.rs documentation will not include private items such as these ones, only public documentation. It might be interesting to have an internal page that adds documentation for all private interfaces as well.

@HalidOdat
Copy link
Member Author

HalidOdat commented Apr 1, 2020

We already have the docs.rs documentation which automatically generates on every release, I don’t understand what the github pages would add? I may be missing the idea here though.

docs.rs documentation will not include private items such as these ones, only public documentation. It might be interesting to have an internal page that adds documentation for all private interfaces as well.

For newcomers to the project that want to contribute, it would be great!

@Razican
Copy link
Member

Razican commented Apr 1, 2020

About this particular PR, I would put the links without showing the full URL, as we did for the first case, since URLs are too long in my opinion. I'm doing the same with the new parser subdivision.

boa/src/syntax/ast/op.rs Outdated Show resolved Hide resolved
boa/src/syntax/ast/op.rs Outdated Show resolved Hide resolved
boa/src/syntax/ast/op.rs Outdated Show resolved Hide resolved
@Razican
Copy link
Member

Razican commented Apr 1, 2020

I would like to document all of syntax module before merging.

About this, I would leave the parser module for a new PR. I'm currently working on dividing the parser in smaller parsers in new modules in the parser_modularization branch, and I'm adding full documentation there.

@HalidOdat HalidOdat closed this Apr 1, 2020
@HalidOdat HalidOdat reopened this Apr 1, 2020
@HalidOdat
Copy link
Member Author

I don't think pos needs more documentation. any thoughts?

@Razican
Copy link
Member

Razican commented Apr 1, 2020

I don't think pos needs more documentation. any thoughts?

I would add some module-level documentation in both op and pos. Something like:

//! This module implements the `Pos` structure, which represents a position in the source code.

@HalidOdat
Copy link
Member Author

HalidOdat commented Apr 2, 2020

@Razican and @jasonwilliams I would like your feedback on this.
The documentation I put in Keyword should I put it in Node?
If so what should i put in Keyword?
Or should I write the same documentation in Node as well.

@jasonwilliams
Copy link
Member

Will try and look today

@Razican
Copy link
Member

Razican commented Apr 2, 2020

The documentation I put in Keyword should I put it in Node?

I've been reading the documentation a bit, and maybe it makes more sense in Node, yes.

If so what should i put in Keyword?
Or should I write the same documentation in Node as well.

I wouldn't duplicate documentation, it seems easy to mess up. Maybe we could say something like this:

/// The else keyword.
///
/// This keyword precedes an `else` block related to an `if` block. For more information:
/// - [If `Node` documentation][link_to_if_node]
/// - [MDN documentation][same_link_as_now]
/// - [ECMAScript specification][same_link_as_now]

What do you think?

@HalidOdat
Copy link
Member Author

Thanks for the feedback! That really helped! :)

@jasonwilliams
Copy link
Member

jasonwilliams commented Apr 2, 2020

Have you been running cargo doc --open --no-deps ?
There have been a lot of failures.

I've added missing_doc_code_examples to allowed but there's still some errors
I've also commented out rustdoc from https://github.com/jasonwilliams/boa/blob/master/boa/src/lib.rs#L9 for now to get it to build

We should add cargo doc as one of our checks for PRs

@jasonwilliams
Copy link
Member

I've been looking at this through the rendered doc that cargo doc generated.
I agree with what @Razican suggested

@jasonwilliams jasonwilliams added this to the v0.7.0 - New Parser milestone Apr 4, 2020
@HalidOdat
Copy link
Member Author

Why do we have GetConstField and GetField?. They do the same operation

@jasonwilliams
Copy link
Member

jasonwilliams commented Apr 8, 2020

Why do we have GetConstField and GetField?. They do the same operation

From what i understand they don't.
GetConstField will fetch you the field from the string property of an object

Example:
foo.bar bar is already a string, so it just does a simple lookup

getField will evaluate what's inside the square brackets then fetches the resolved value as a field
foo[bar]

bar could be anything, so it will be evaluated first, then the (string) result of that it used for the look up.

            Node::GetConstField(ref obj, ref field) => {
                let val_obj = self.run(obj)?;
                Ok(val_obj.borrow().get_field_slice(field))
            }
            Node::GetField(ref obj, ref field) => {
                let val_obj = self.run(obj)?;
                let val_field = self.run(field)?;
                Ok(val_obj
                    .borrow()
                    .get_field_slice(&val_field.borrow().to_string()))
            }

getField performs an extra step by evaluating the field, where as in GetConstField we already know its a string, so we don't need to evaluate it.

@HalidOdat
Copy link
Member Author

HalidOdat commented Apr 8, 2020

Interesting. So It is an optimization.

@jasonwilliams
Copy link
Member

Are there any I can start to fill in?

@HalidOdat
Copy link
Member Author

HalidOdat commented Apr 10, 2020

Are there any I can start to fill in?

If you could fill in lexer that would be awesome. :)

@jasonwilliams jasonwilliams removed this from the v0.7.0 - New Parser milestone Apr 12, 2020
@HalidOdat HalidOdat marked this pull request as draft April 12, 2020 18:41
@HalidOdat HalidOdat requested a review from Razican April 26, 2020 12:41
Copy link
Member

@jasonwilliams jasonwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking

@jasonwilliams
Copy link
Member

Looks great!
Thanks for the monsterous effort on that

@HalidOdat HalidOdat merged commit 41cda68 into boa-dev:master Apr 27, 2020
@HalidOdat HalidOdat deleted the better-documentation branch April 27, 2020 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation update documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants